Skip to content

[Repo Assist] Fix thread-safety races in member-wrapper caches (issue #481)#482

Open
github-actions[bot] wants to merge 5 commits intomasterfrom
repo-assist/fix-thread-safety-lazy-caches-481-a00dbff9b73cb3e8
Open

[Repo Assist] Fix thread-safety races in member-wrapper caches (issue #481)#482
github-actions[bot] wants to merge 5 commits intomasterfrom
repo-assist/fix-thread-safety-lazy-caches-481-a00dbff9b73cb3e8

Conversation

@github-actions
Copy link
Contributor

🤖 This is an automated pull request from Repo Assist.

Fixes #481InvalidOperationException / NullReferenceException when the F# compiler accesses type-provider types from multiple parallel compilation threads.

Root cause

PR #471 introduced lazy caches (ctorDefs/methDefs/fieldDefs/eventDefs/propDefs/nestedDefs) in TargetTypeDefinition so member-wrapper objects are allocated once and reused. When the compiler invokes GetConstructors/GetMethods/etc. from multiple threads on the same type, several underlying caches that were never designed for concurrent access are hit simultaneously:

Site Problem
ILMethodDefs.getmap() / ILTypeDefs.getmap() / ILExportedTypesAndForwarders.getmap() mutable lmap = null checked without a lock. Thread A sets lmap to a new Dictionary and starts filling it; Thread B sees the non-null value and reads from it while Thread A is writing → InvalidOperationException.
mkCacheInt32 / mkCacheGeneric (binary-reader row caches) Same unsynchronised ref null / Dictionary pattern across all 8 per-reader caches.
TxTable(T).Get Dictionary(int,T) written without any lock; concurrent type-resolution calls (via cached MethodInfo/ConstructorInfotxILTypeReftxTable.Get) from two threads can collide.

Fix

  • ILMethodDefs / ILTypeDefs / ILExportedTypesAndForwarders – add a syncObj per instance; build lmap inside lock syncObj so the dictionary is fully populated before any reader sees it. Subsequent calls acquire the lock, see the already-set field, and return immediately.
  • mkCacheInt32 / mkCacheGeneric – each closure now owns a syncObj and wraps every TryGetValue + write pair in lock.
  • TxTable(T) – backed by ConcurrentDictionary(int, Lazy<T)>. GetOrAdd races safely; the Lazy(T) wrapper (using F#'s default ExecutionAndPublication mode) ensures the factory is called at most once per token, preserving the identity-equality guarantee that TxTable provides.

Test status

New regression test added: TargetTypeDefinition member-wrapper caches are thread-safe under parallel access — 8 threads × 50 iterations, each calling all six GetXxx methods on the same TargetTypeDefinition concurrently.

Passed!  - Failed: 0, Passed: 118, Skipped: 0, Total: 118 (net8.0)

All pre-existing tests continue to pass.

Generated by Repo Assist for issue #481 ·

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@d1d884596e62351dd652ae78465885dd32f0dd7d

Closes #481

Root cause: PR #471 added lazy caches (ctorDefs/methDefs/fieldDefs/etc.)
in TargetTypeDefinition so wrapper objects are allocated once and shared
across all GetConstructors/GetMethods/etc. calls.  When the F# compiler
invokes these from multiple threads concurrently, the lazies can be forced
concurrently, and the underlying non-thread-safe caches race:

* ILMethodDefs.getmap() / ILTypeDefs.getmap() / ILExportedTypesAndForwarders.getmap()
  used a mutable null-check pattern without synchronisation.  One thread
  sets lmap to a new Dictionary and starts filling it; a second thread sees
  the non-null lmap and reads it while the first is still writing ->
  InvalidOperationException.
* mkCacheInt32 / mkCacheGeneric (binary-reader caches) had the same
  unsynchronised ref-null pattern.
* TxTable<T>.Get wrote to Dictionary<int,T> without a lock; concurrent
  type-resolution calls (txILTypeRef -> txTable.Get) from shared cached
  MethodInfo/ConstructorInfo objects could collide.

Fixes:
* ILMethodDefs / ILTypeDefs / ILExportedTypesAndForwarders: build lmap
  inside lock syncObj so the dictionary is fully populated before any
  reader can see it.  Subsequent calls acquire the lock, check the
  already-set field and return immediately (single branch).
* mkCacheInt32 / mkCacheGeneric: each cache now holds its own lock object
  and protects every TryGetValue/set_Item pair.
* TxTable<T>: backed by ConcurrentDictionary<int, Lazy<T>> so that
  concurrent GetOrAdd calls for the same token race safely, with Lazy<T>
  guaranteeing the factory runs exactly once per token.

Adds a thread-safety regression test: 8 threads × 50 iterations each
calling GetConstructors/GetMethods/GetFields/GetProperties/GetEvents/
GetNestedTypes on the same TargetTypeDefinition simultaneously.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses thread-safety races in the SDK’s metadata-backed reflection wrappers that can be accessed concurrently by the F# compiler during parallel compilation, preventing intermittent InvalidOperationException/NullReferenceException failures (issue #481).

Changes:

  • Adds synchronization to lazy dictionary materialization in ILMethodDefs, ILTypeDefs, and ILExportedTypesAndForwarders.
  • Makes metadata row caches (mkCacheInt32/mkCacheGeneric) safe under concurrent access by guarding cache reads/writes.
  • Reworks TxTable<'T> to use ConcurrentDictionary<int, Lazy<'T>> and adds a regression test exercising concurrent member enumeration on a generated target type.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/ProvidedTypes.fs Adds locking / concurrent structures to prevent concurrent mutations of internal caches used during metadata-backed reflection.
tests/BasicGenerativeProvisionTests.fs Adds a regression test that concurrently calls GetConstructors/GetMethods/etc. on the same generated TargetTypeDefinition.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sergey-tihon
Copy link
Member

@copilot i don't like this implementation, it is too complicated. rework it using ConcurrentDictionary

Copy link

Copilot AI commented Mar 22, 2026

@sergey-tihon I've opened a new pull request, #486, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 3 commits March 22, 2026 08:05
`mkCacheInt32` and `mkCacheGeneric` were using `Dictionary` + `lock
syncObj`, serializing all cache access including expensive metadata
decode work.

## Changes

- **`mkCacheInt32` / `mkCacheGeneric`**: replaced `Dictionary` + `lock`
with `ConcurrentDictionary` using `TryGetValue` / `TryAdd`. No locks, no
`syncObj`.

```fsharp
let mkCacheInt32 lowMem _infile _nm _sz =
    if lowMem then (fun f x -> f x) else
    let cache = ConcurrentDictionary<int32, _>()
    fun f (idx:int32) ->
        match cache.TryGetValue idx with
        | true, v -> v
        | false, _ ->
            let v = f idx
            cache.TryAdd(idx, v) |> ignore
            cache.[idx]
```

`GetOrAdd(key, factory)` was not usable here — F# type inference cannot
disambiguate it from `GetOrAdd(key, value)` when the value type is an
unconstrained generic. `TryAdd` has no overloads and sidesteps the
issue. On a concurrent cache miss, `f idx` may be computed twice (both
`GetOrAdd` with factory and this pattern share that behaviour per the
.NET docs), which is acceptable since these factories are pure metadata
reads.

<!-- START COPILOT CODING AGENT TIPS -->
---

⌨️ Start Copilot coding agent tasks without leaving your editor —
available in [VS Code](https://gh.io/cca-vs-code-docs), [Visual
Studio](https://gh.io/cca-visual-studio-docs), [JetBrains
IDEs](https://gh.io/cca-jetbrains-docs) and
[Eclipse](https://gh.io/cca-eclipse-docs).

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: sergey-tihon <1197905+sergey-tihon@users.noreply.github.com>
…ches

The ILMethodDefs, ILTypeDefs and ILExportedTypesAndForwarders caches are
build-once-read-many, so a simple lazy value provides thread-safe init
without introducing mutable fields, lock objects or ConcurrentDictionary
overhead.
@sergey-tihon sergey-tihon self-requested a review March 22, 2026 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ILTypeBuilder lazy member-wrapper caches may race under parallel F# compilation

3 participants